Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

thriftbp: Metrics cleanup #581

Merged
merged 1 commit into from
Nov 2, 2022
Merged

Conversation

fishy
Copy link
Member

@fishy fishy commented Nov 1, 2022

Remove statsd metrics, and rename some prometheus metrics to comply with prometheus naming convention. For renamed prometheus metrics, temporarily emitting both old and new, but mark old ones as deprecated to be removed later.

Also remove some statsd only features:

  1. CountedServerTransport is marked as deprecated and no longer used by other code.
  2. In ReportPayloadSizeMetrics server middleware, the rate arg is deprecated and the prometheus metrics is always reported.
  3. ClientPoolConfig.MetricsTags & ClientPoolConfig.ReportPoolStats are deprecated. The pool stats are always reported as prometheus metrrics.

@fishy fishy requested a review from a team as a code owner November 1, 2022 21:26
@fishy fishy requested review from kylelemons, pacejackson and ghirsch-reddit and removed request for a team November 1, 2022 21:26
Remove statsd metrics, and rename some prometheus metrics to comply with
prometheus naming convention. For renamed prometheus metrics,
temporarily emitting both old and new, but mark old ones as deprecated
to be removed later.

Also remove some statsd only features:

1. CountedServerTransport is marked as deprecated and no longer used by
   other code.
2. In ReportPayloadSizeMetrics server middleware, the rate arg is
   deprecated and the prometheus metrics is always reported.
3. ClientPoolConfig.MetricsTags & ClientPoolConfig.ReportPoolStats are
   deprecated. The pool stats are always reported as prometheus
   metrrics.
Copy link
Contributor

@ghirsch-reddit ghirsch-reddit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably good to get a ✅ from someone more familiar with the code, but this all looks great to me! Always great to see us ✂️ and clean up old stuff

}, panicRecoverLabels)

panicRecoverCounter = promauto.With(prometheusbpint.GlobalRegistry).NewCounterVec(prometheus.CounterOpts{
Name: "thriftbp_server_recovered_panics_total",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically (according to some dictionaries) "panic" is uncountable as a noun, but I think we pretty much use it as countable noun in go.

fishy added a commit to fishy/baseplate.go that referenced this pull request Nov 1, 2022
Remove/deprecate statsd metrics, and rename an httpbp prometheus metrics
similar to the thriftbp one in
reddit#581.
fishy added a commit to fishy/baseplate.go that referenced this pull request Nov 1, 2022
Remove/deprecate statsd metrics, and rename an httpbp prometheus metrics
similar to the thriftbp one in
reddit#581.
@fishy fishy merged commit bcdec55 into reddit:master Nov 2, 2022
@fishy fishy deleted the thriftbp-metrics-cleanup branch November 2, 2022 18:24
fishy added a commit to fishy/baseplate.go that referenced this pull request Nov 2, 2022
Remove/deprecate statsd metrics, and rename an httpbp prometheus metrics
similar to the thriftbp one in
reddit#581.
fishy added a commit that referenced this pull request Nov 2, 2022
Remove/deprecate statsd metrics, and rename an httpbp prometheus metrics
similar to the thriftbp one in
#581.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants